Skip to content

SEC-283 - Preserve whitespace formatting for comments in property files#252

Merged
Arvind Thirunarayanan (arvindth) merged 2 commits into
confluentinc:masterfrom
arvindth:ath-SEC-283-preserveFormatting
Aug 19, 2019
Merged

SEC-283 - Preserve whitespace formatting for comments in property files#252
Arvind Thirunarayanan (arvindth) merged 2 commits into
confluentinc:masterfrom
arvindth:ath-SEC-283-preserveFormatting

Conversation

@arvindth

@arvindth Arvind Thirunarayanan (arvindth) commented Aug 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Changes:

  • Update to use the confluent fork of the underlying go library for property file parsing & writing
  • Remove previous workaround to add/remove a final dummy key/value pair to preserve trailing comments in the input property files. This is now handled natively by the updated library.
  • Update the call to read/write property files to use the new preserveFormatting option.

Note:
Upstream PR - magiconair/properties#38

  • If it gets accepted upstream, cli should be reverted to point back to the upstream library and the confluentinc/properties fork should be deleted.

@arvindth

Arvind Thirunarayanan (arvindth) commented Aug 14, 2019

Copy link
Copy Markdown
Member Author

Yeva Byzek (@ybyzek), I've tested this change against some variations of properties files based on your original description in the ticket. However, as the original reporter I would request that you try out this branch on some properties files to ensure that it meets the ask from the ticket.

@ybyzek

Yeva Byzek (ybyzek) commented Aug 14, 2019

Copy link
Copy Markdown

Arvind Thirunarayanan (@arvindth) awesome work! However, I'm very slammed and won't have time to validate this. Please validate with QA, PM, or others...please don't block on me. Thanks!

@arvindth

Arvind Thirunarayanan (arvindth) commented Aug 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@confluentinc/cli, while reviewing this, note that this depends on an update to the underlying magiconair/properties library, which I've done on a confluentinc fork (confluentinc/properties#1). Please let me know if you think the fork's changes should also go through a PR review.

@DABH

David Hyde (DABH) commented Aug 19, 2019

Copy link
Copy Markdown
Contributor

Arvind Thirunarayanan (@arvindth) no need for us reviewing that lib, that is between you and Frank Schröder (@magiconair) : )

DABH
David Hyde (DABH) approved these changes Aug 19, 2019
View reviewed changes

@DABH David Hyde (DABH) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arvindth Arvind Thirunarayanan (arvindth) merged commit 19adbdf into confluentinc:master Aug 19, 2019
@arvindth Arvind Thirunarayanan (arvindth) deleted the ath-SEC-283-preserveFormatting branch August 19, 2019 18:13
This was referenced Aug 19, 2019
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ybyzek Yeva Byzek (ybyzek) Awaiting requested review from ybyzek
1 more reviewer
@DABH David Hyde (DABH) DABH approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@arvindth @ybyzek @DABH